Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

obs-outputs: Add eRTMP/eFLV multitrack support #10500

Merged

Conversation

palana
Copy link
Contributor

@palana palana commented Apr 9, 2024

Description

Add eRTMP multitrack video support for the rtmp output and "full" eFLV support for the FLV output (both additional video codecs and multitrack video)

Motivation and Context

This allows transmitting multiple video streams within the same RTMP connection; it's used for Twitch Enhanced Broadcasting to allow multiple renditions to be sent through a single RTMP connection, helping ensure that renditions remain aligned

How Has This Been Tested?

It's being used in the Twitch Enhanced Broadcasting Beta

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@palana palana force-pushed the ruwen/upstream-rtmp-changes branch from 34c96bf to 0492d45 Compare April 11, 2024 13:35
@WizardCM WizardCM added the New Feature New feature or plugin label Apr 13, 2024
@palana palana force-pushed the ruwen/upstream-rtmp-changes branch 2 times, most recently from 4391905 to 6746e20 Compare April 15, 2024 11:09
@RytoEX
Copy link
Member

RytoEX commented Apr 15, 2024

With #10494 merged, this can be rebased and undrafted.

@palana palana force-pushed the ruwen/upstream-rtmp-changes branch from 6746e20 to 667ab1c Compare April 16, 2024 10:26
@palana palana marked this pull request as ready for review April 16, 2024 10:26
@palana
Copy link
Contributor Author

palana commented Apr 16, 2024

With #10494 merged, this can be rebased and undrafted.

@RytoEX: done

@RytoEX RytoEX self-assigned this Apr 16, 2024
Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nits.

plugins/obs-outputs/flv-mux.c Outdated Show resolved Hide resolved
plugins/obs-outputs/flv-output.c Outdated Show resolved Hide resolved
plugins/obs-outputs/flv-output.c Outdated Show resolved Hide resolved
plugins/obs-outputs/flv-output.c Outdated Show resolved Hide resolved
plugins/obs-outputs/flv-output.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@Lain-B Lain-B left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outside of the ryan/pat nitpicks this has my approval. An alternative for the assert might be like, bcrash() or something. (If it's not just a debug situation at least)

@palana palana force-pushed the ruwen/upstream-rtmp-changes branch from d7cd61b to 9423d35 Compare April 19, 2024 10:53
@palana
Copy link
Contributor Author

palana commented Apr 19, 2024

@RytoEX: nits should be addressed and squashed (and rebased)

Copy link
Member

@derrod derrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple smaller things, it would be good if you could also adjust init_send() to correctly accumulate bitrate from all video encoders rather than just the primary one, that should fix the issues peopole have seen with the network enhancement options, but that's not a blocker for now.

plugins/obs-outputs/rtmp-stream.c Outdated Show resolved Hide resolved
plugins/obs-outputs/rtmp-stream.c Outdated Show resolved Hide resolved
plugins/obs-outputs/rtmp-stream.c Outdated Show resolved Hide resolved
plugins/obs-outputs/flv-mux.c Outdated Show resolved Hide resolved
Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nits. I'll probably pass after this round.

plugins/obs-outputs/flv-output.c Outdated Show resolved Hide resolved
plugins/obs-outputs/flv-output.c Outdated Show resolved Hide resolved
plugins/obs-outputs/flv-output.c Outdated Show resolved Hide resolved
plugins/obs-outputs/rtmp-stream.c Outdated Show resolved Hide resolved
@palana palana force-pushed the ruwen/upstream-rtmp-changes branch 3 times, most recently from 62550bf to 26a18c4 Compare April 29, 2024 11:37
@palana palana force-pushed the ruwen/upstream-rtmp-changes branch 2 times, most recently from 4154cfb to ba55e1d Compare May 2, 2024 13:23
@RytoEX
Copy link
Member

RytoEX commented May 2, 2024

Off-thread discussion indicates that this looks good. Squash the commits appropriately and we'll give it a (hopefully) final pass.

@palana palana force-pushed the ruwen/upstream-rtmp-changes branch from ba55e1d to 552f30a Compare May 3, 2024 10:12
@palana
Copy link
Contributor Author

palana commented May 3, 2024

Off-thread discussion indicates that this looks good. Squash the commits appropriately and we'll give it a (hopefully) final pass.

@RytoEX: squashed and rebased

kc5nra and others added 3 commits May 3, 2024 16:23
This will be initialized to 0 in various cases, so let's make that
a valid enum value (even if it's not valid in rtmp?)
@palana palana force-pushed the ruwen/upstream-rtmp-changes branch from 552f30a to b0799b8 Compare May 3, 2024 14:23
@RytoEX RytoEX added this to the OBS Studio (Next Version) milestone May 3, 2024
@RytoEX RytoEX merged commit 64caf04 into obsproject:master May 3, 2024
14 checks passed
@palana palana deleted the ruwen/upstream-rtmp-changes branch May 3, 2024 19:51
@RytoEX RytoEX mentioned this pull request May 4, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Feature New feature or plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants